Skip to content

Fade to black - #106

Merged
wsanchez merged 15 commits into
masterfrom
black
Apr 14, 2020
Merged

wsanchez merged 15 commits into
masterfrom
black

Conversation

@wsanchez

@wsanchez wsanchez commented Jan 25, 2020 •

Copy link
Copy Markdown
Contributor

This PR sets up the black linter.

The current branch does not actually run black in the default tox config, or in CI, so that the reviewer can see what changes are made before we reformat the code base.

To run black, use this command: tox -e black. It will fail, since the code is not yet reformatted to comply with black's conventions.

To test out the reformatting, run tox -e black-reformat. At this point, running black (per above) should succeed.

Before merging, we need to (1) reformat the code, (2) add black to the default tox environment list (it's commented out for now) and (3) add the black tox environment to the CI config (4) remove W504 from the flake8 ignore list, as it won't be needed post-reformatting.

@wsanchez
wsanchez requested a review from mahmoud January 25, 2020 23:30
@wsanchez wsanchez self-assigned this Jan 25, 2020
@codecov-io

codecov-io commented Jan 25, 2020 •

Copy link
Copy Markdown

Codecov Report

Merging #106 into master will not change coverage by %.
The diff coverage is 98.57%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #106   +/-   ##
=======================================
  Coverage   98.55%   98.55%           
=======================================
  Files          10       10           
  Lines        1595     1595           
  Branches      187      187           
=======================================
  Hits         1572     1572           
  Misses         12       12           
  Partials       11       11           
Impacted Files Coverage Δ
src/hyperlink/test/common.py 100.00% <ø> (ø)
src/hyperlink/_url.py 96.91% <95.59%> (ø)
src/hyperlink/_socket.py 100.00% <100.00%> (ø)
src/hyperlink/test/test_common.py 100.00% <100.00%> (ø)
src/hyperlink/test/test_decoded_url.py 100.00% <100.00%> (ø)
src/hyperlink/test/test_parse.py 100.00% <100.00%> (ø)
src/hyperlink/test/test_scheme_registration.py 100.00% <100.00%> (ø)
src/hyperlink/test/test_url.py 99.82% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 563980a...cfb18a6. Read the comment docs.

Comment thread MANIFEST.in Resolved
Comment thread pyproject.toml Resolved
Comment thread tox.ini Resolved
Comment thread tox.ini Resolved
Comment thread tox.ini Resolved
Comment thread tox.ini Resolved
Comment thread tox.ini Resolved
Comment thread tox.ini Resolved
@wsanchez wsanchez mentioned this pull request Apr 2, 2020
wsanchez added 2 commits April 2, 2020 08:03
* master:
  [requires.io] dependency update
  [requires.io] dependency update
  Spiff up the tox config a bit more
  There is no requirements-test.txt file in the source tree.
  Archor a few paths to the root of the source tree. Minor re-ordering.
  per CR: rephrase gibberish test docstring
  per CR: add https:/, enumerate the cases
  per CR: match __init__
  match __init__ doc
  per CR: explain in much more detail
  <79
  per CR: make the test a little more thorough, improve docstring
  fix up inconsistencies in parsing & textual representation of 'rooted' and 'uses_netloc'

# Conflicts:
#	.gitignore
#	MANIFEST.in
#	tox.ini
@wsanchez

wsanchez commented Apr 6, 2020

Copy link
Copy Markdown
Contributor Author

Restarting builds in hopes that codecov wakes up

@wsanchez

Copy link
Copy Markdown
Contributor Author

@mahmoud if you get some time for a review, this one would be logistically nice, as this is a difficult branch to merge forward repeatedly due to black touching a lot of code, so there are frequent conflicts.

@mahmoud mahmoud left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main concern with Black is how it affects contributor workflow. Maybe we could use a doc on that?

After we merge this, contributors should run tox -e black-reformat before committing?

Also, don't forget to remove that W504.

Comment thread tox.ini
skip_install = {[testenv:black]skip_install}
deps = {[testenv:black]deps}
commands = {[testenv:black]commands}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this works because the lint args are absent?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, {env:BLACK_LINT_ARGS:} evaluates to "" in the black-reformat environment.

@wsanchez

Copy link
Copy Markdown
Contributor Author

Thanks for the W504 reminder; I had let that one slip.

And yes, you should run tox -e black-reformat before committing, or at least before pushing. You could also just run black directly if you have it installed; some editors will run black for you, for example.

I do use the Tox environment, because I know that'll work even if I don't have black installed, and it ensured I use the right version in the right way, but YYMV.

@wsanchez
wsanchez merged commit 0153229 into master Apr 14, 2020
@wsanchez
wsanchez deleted the black branch April 14, 2020 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants